feat(ev-deployer): part 1 - add ev-deployer CLI for genesis contract allocation#167
Open
feat(ev-deployer): part 1 - add ev-deployer CLI for genesis contract allocation#167
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as ev-deployer CLI
participant Config as Config Loader
participant Contracts as Contract Builders
participant Genesis as Genesis Builder
participant JSON as JSON Output
participant Node as Local Node
User->>CLI: genesis --config devnet.toml --merge-into genesis.json
CLI->>Config: DeployConfig::load(path)
Config->>Config: validate()
Config-->>CLI: DeployConfig
CLI->>Contracts: admin_proxy::build(&config)
Contracts->>Contracts: encode owner → storage slot 0
Contracts-->>CLI: GenesisContract
CLI->>Contracts: fee_vault::build(&config)
Contracts->>Contracts: encode slots 0–6, pack owner+domain, default bridge_share_bps
Contracts-->>CLI: GenesisContract
CLI->>Genesis: build_alloc(config)
Genesis->>Genesis: assemble alloc entries (balance, code, storage)
Genesis-->>CLI: alloc JSON Value
CLI->>Genesis: merge_into(path, force)
Genesis->>Genesis: read existing genesis.json, merge or overwrite entries
Genesis-->>CLI: merged alloc JSON Value
CLI->>JSON: serialize pretty JSON
JSON-->>CLI: file/stdout written
CLI-->>User: status
User->>Node: RPC: eth_getCode / eth_getStorageAt
Node-->>User: bytecode / storage values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @randygrok's task —— View job Code Review: EV Deployer CLI ToolI've conducted a thorough review of the ev-deployer implementation. Overall, this is a solid CLI tool with good architecture and comprehensive testing. Here are my findings: ✅ StrengthsCode Quality & Architecture
Security
Testing
|
randygrok
changed the title
randygrok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ev_deployer.yml:
- Around line 6-13: The pull_request path filters in ev_deployer.yml only
include 'contracts/src/**', 'contracts/foundry.toml', and 'bin/ev-deployer/**',
so edits to root Cargo.toml, Cargo.lock, or the workflow itself won't trigger EV
Deployer CI and can break cargo test -p ev-deployer; update the
pull_request.paths block (the pull_request.paths entry in
.github/workflows/ev_deployer.yml) to also include the root-level files like
'Cargo.toml', 'Cargo.lock', and the workflow file (e.g.
'.github/workflows/ev_deployer.yml') so changes to those files will trigger the
workflow.
In `@bin/ev-deployer/README.md`:
- Around line 86-90: The README's description of the merge behavior is
misleading: update the text around the ev-deployer genesis command (referencing
the --merge-into and --output flags) to remove the phrase "modifies the `alloc`
field in-place" and explicitly state that the CLI does not mutate the original
file but writes the merged result to the path provided by --output (or to stdout
if --output is omitted); ensure the example command and accompanying sentence
clearly say the original genesis.json is unchanged and the merged output is
written to genesis-out.json (or stdout).
In `@bin/ev-deployer/src/config.rs`:
- Around line 83-101: The validate() method currently doesn't ensure contract
addresses are unique, allowing admin_proxy.address == fee_vault.address which
later causes silent alloc overwrites; add a check in validate() that if both
self.contracts.admin_proxy and self.contracts.fee_vault are Some, their .address
fields must not be equal and fail fast (use eyre::ensure or eyre::bail) with a
clear error like "admin_proxy.address must not equal fee_vault.address"; update
the validate() function to perform this uniqueness assertion alongside the
existing owner and bridge_share_bps checks.
In `@bin/ev-deployer/src/contracts/admin_proxy.rs`:
- Around line 68-73: The current normalization of forge output (building
forge_hex from output.stdout) uses unwraps and will panic on invalid UTF-8 or if
the "0x" prefix is missing; change it to handle errors safely by first
converting output.stdout with String::from_utf8 and propagating or logging the
error instead of unwrap, then perform trimming and lowercase on the resulting
string and remove a leading "0x" non-panically (e.g., use
strip_prefix("0x").unwrap_or(&s) or check starts_with and slice) to produce
forge_hex; update the code that assigns forge_hex (the variable constructed from
output.stdout) to return a Result or fallback value rather than panicking.
In `@bin/ev-deployer/src/contracts/fee_vault.rs`:
- Around line 171-176: The current normalization of forge output into forge_hex
uses multiple unwraps and can panic if the output is not valid UTF-8 or if the
"0x" prefix is missing; change it to a tolerant normalization that avoids
unwraps by using String::from_utf8_lossy on output.stdout and
trim_start_matches("0x") instead of strip_prefix/unwrap, e.g. build forge_hex
from
String::from_utf8_lossy(&output.stdout).trim().trim_start_matches("0x").to_lowercase()
so it safely handles missing "0x" and invalid UTF-8.
In `@bin/ev-deployer/src/main.rs`:
- Around line 71-75: When --merge-into is provided but no explicit --output is
given, the merged genesis currently only goes to stdout; change the control flow
so that when merge_into is Some(genesis_path) and output is None you persist the
merged result back to genesis_path. Concretely, after calling
genesis::merge_into(&cfg, genesis_path, force) (or using the merged value from
genesis::build_alloc when merging), write the resulting genesis bytes/string to
the same genesis_path (or call an existing genesis write helper) instead of only
printing to stdout; update the branch that handles merge_into (and the similar
branch around lines 79–84) to perform that write when output is unspecified.
In `@bin/ev-deployer/tests/e2e_genesis.sh`:
- Around line 35-38: The curl RPC calls that post JSON-RPC using RPC_URL with
variables method and params are currently blocking; update both curl invocations
(the ones that build the JSON payload with "$RPC_URL" and -d
"{\"jsonrpc\":\"2.0\",\"method\":\"$method\",\"params\":$params,\"id\":1}" and
the similar second call) to include connection and overall timeouts (for example
--connect-timeout 5 --max-time 10) so a stalled node/socket won’t hang CI; keep
the existing headers and piping to python3 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 442c5b5a-2b3c-4deb-af81-3a72dbdffa67
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ev_deployer.ymlCargo.tomlbin/ev-deployer/Cargo.tomlbin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/admin_proxy.rsbin/ev-deployer/src/contracts/fee_vault.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.shcontracts/foundry.tomljustfile
Comment on lines
+68
to
+73
| let forge_hex = String::from_utf8(output.stdout) | ||
| .unwrap() | ||
| .trim() | ||
| .strip_prefix("0x") | ||
| .unwrap() | ||
| .to_lowercase(); |
There was a problem hiding this comment.
Avoid panic on forge output normalization.
Line 71/72 assumes a 0x prefix and will panic on unexpected forge output shape. Prefer a non-panicking normalization path.
Suggested fix
- let forge_hex = String::from_utf8(output.stdout)
- .unwrap()
- .trim()
- .strip_prefix("0x")
- .unwrap()
- .to_lowercase();
+ let forge_hex = String::from_utf8(output.stdout)
+ .unwrap()
+ .trim()
+ .trim_start_matches("0x")
+ .to_lowercase();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let forge_hex = String::from_utf8(output.stdout) | |
| .unwrap() | |
| .trim() | |
| .strip_prefix("0x") | |
| .unwrap() | |
| .to_lowercase(); | |
| let forge_hex = String::from_utf8(output.stdout) | |
| .unwrap() | |
| .trim() | |
| .trim_start_matches("0x") | |
| .to_lowercase(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/contracts/admin_proxy.rs` around lines 68 - 73, The
current normalization of forge output (building forge_hex from output.stdout)
uses unwraps and will panic on invalid UTF-8 or if the "0x" prefix is missing;
change it to handle errors safely by first converting output.stdout with
String::from_utf8 and propagating or logging the error instead of unwrap, then
perform trimming and lowercase on the resulting string and remove a leading "0x"
non-panically (e.g., use strip_prefix("0x").unwrap_or(&s) or check starts_with
and slice) to produce forge_hex; update the code that assigns forge_hex (the
variable constructed from output.stdout) to return a Result or fallback value
rather than panicking.
Comment on lines
+171
to
+176
| let forge_hex = String::from_utf8(output.stdout) | ||
| .unwrap() | ||
| .trim() | ||
| .strip_prefix("0x") | ||
| .unwrap() | ||
| .to_lowercase(); |
There was a problem hiding this comment.
Use non-panicking forge hex normalization in test.
Line 174/175 can panic if forge output unexpectedly omits 0x. Prefer tolerant normalization.
Suggested fix
- let forge_hex = String::from_utf8(output.stdout)
- .unwrap()
- .trim()
- .strip_prefix("0x")
- .unwrap()
- .to_lowercase();
+ let forge_hex = String::from_utf8(output.stdout)
+ .unwrap()
+ .trim()
+ .trim_start_matches("0x")
+ .to_lowercase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/contracts/fee_vault.rs` around lines 171 - 176, The
current normalization of forge output into forge_hex uses multiple unwraps and
can panic if the output is not valid UTF-8 or if the "0x" prefix is missing;
change it to a tolerant normalization that avoids unwraps by using
String::from_utf8_lossy on output.stdout and trim_start_matches("0x") instead of
strip_prefix/unwrap, e.g. build forge_hex from
String::from_utf8_lossy(&output.stdout).trim().trim_start_matches("0x").to_lowercase()
so it safely handles missing "0x" and invalid UTF-8.
Comment on lines
+71
to
+75
| let result = if let Some(ref genesis_path) = merge_into { | ||
| genesis::merge_into(&cfg, genesis_path, force)? | ||
| } else { | ||
| genesis::build_alloc(&cfg) | ||
| }; |
There was a problem hiding this comment.
--merge-into without --output drops merged result to stdout only.
When users pass --merge-into alone, the merged genesis is not written back to the source file. That makes merge behavior easy to misuse and lose.
Suggested fix (persist merge target by default)
- if let Some(ref out_path) = output {
- std::fs::write(out_path, &json)?;
- eprintln!("Wrote alloc to {}", out_path.display());
- } else {
- println!("{json}");
- }
+ if let Some(target_path) = output.as_ref().or(merge_into.as_ref()) {
+ std::fs::write(target_path, &json)?;
+ eprintln!("Wrote alloc to {}", target_path.display());
+ } else {
+ println!("{json}");
+ }Also applies to: 79-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/main.rs` around lines 71 - 75, When --merge-into is
provided but no explicit --output is given, the merged genesis currently only
goes to stdout; change the control flow so that when merge_into is
Some(genesis_path) and output is None you persist the merged result back to
genesis_path. Concretely, after calling genesis::merge_into(&cfg, genesis_path,
force) (or using the merged value from genesis::build_alloc when merging), write
the resulting genesis bytes/string to the same genesis_path (or call an existing
genesis write helper) instead of only printing to stdout; update the branch that
handles merge_into (and the similar branch around lines 79–84) to perform that
write when output is unspecified.
- Expand CI workflow path triggers to include Cargo.toml, Cargo.lock,
and the workflow file itself
- Fix README merge behavior description (not in-place)
- Validate contract addresses are unique in config
- Use trim_start_matches("0x") instead of strip_prefix().unwrap() in
bytecode verification tests
- Add curl timeouts to e2e test RPC calls to prevent CI hangs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/ev-deployer/src/contracts/fee_vault.rs (1)
171-175:⚠️ Potential issue | 🟡 MinorAvoid panic in forge output decoding path.
Line 171 still uses
String::from_utf8(output.stdout).unwrap(), which can panic on unexpected bytes. Use lossy decoding to keep this parity test tolerant.Proposed fix
- let forge_hex = String::from_utf8(output.stdout) - .unwrap() + let forge_hex = String::from_utf8_lossy(&output.stdout) .trim() .trim_start_matches("0x") .to_lowercase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/src/contracts/fee_vault.rs` around lines 171 - 175, The current forge_hex construction uses String::from_utf8(output.stdout).unwrap() which can panic on invalid UTF-8; change the decoding to a lossy form (e.g. use String::from_utf8_lossy on output.stdout) before trim/trim_start_matches/to_lowercase so the parity test tolerates unexpected bytes and avoids panics when building forge_hex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/ev-deployer/src/contracts/fee_vault.rs`:
- Around line 171-175: The current forge_hex construction uses
String::from_utf8(output.stdout).unwrap() which can panic on invalid UTF-8;
change the decoding to a lossy form (e.g. use String::from_utf8_lossy on
output.stdout) before trim/trim_start_matches/to_lowercase so the parity test
tolerates unexpected bytes and avoids panics when building forge_hex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7550381-d587-49ad-b622-1fca28fe6555
📒 Files selected for processing (6)
.github/workflows/ev_deployer.ymlbin/ev-deployer/README.mdbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/admin_proxy.rsbin/ev-deployer/src/contracts/fee_vault.rsbin/ev-deployer/tests/e2e_genesis.sh
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ev_deployer.yml
- bin/ev-deployer/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/ev-deployer/src/config.rs
- bin/ev-deployer/src/contracts/admin_proxy.rs
- bin/ev-deployer/tests/e2e_genesis.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #119
Implements Part 1 (Core) of EV Deployer — a CLI tool for generating genesis alloc entries for ev-reth contracts via declarative TOML configuration.
The tool reads a TOML config file, encodes contract bytecode and storage state, and generates Ethereum-compatible genesis alloc JSON. Supports merging into existing genesis files with collision detection. Currently handles AdminProxy and FeeVault contracts.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores